fix: address code review issues — config.R, duplicate code, misleading names - #1
Open
mugpeng wants to merge 2 commits into
Open
fix: address code review issues — config.R, duplicate code, misleading names#1mugpeng wants to merge 2 commits into
mugpeng wants to merge 2 commits into
Conversation
…g names - Add config.R.example with all required parameters documented - Add scripts/utils.R with shared utility functions (normalize_gene, normalize_tissue, read_gmt_genes, load_rda_list, average_precision_grouped) - Remove duplicate utility functions from 01/02/03; source utils.R - Rename RankProduct_missing0 -> RankGeometricMean_missing0 (Bug #3) - Rename winner_single_only_fraction -> winner_no_both_fraction (Bug #2) - 02/03 now source config.R instead of hardcoding paths/parameters - Add .gitignore for R artifacts and large regenerable outputs - Add clear error messages when config.R is missing, pointing to config.R.example Addresses Bugs GangLiLab#1-#5, #7, #10 from the code review. Remaining issues (#6, #8, #9) are design choices left to authors.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi authors,
Thank you for sharing the COMPASS benchmark scripts. After carefully reviewing the code, I've put together a PR addressing the bugs and code quality issues identified in a recent code review. All changes are backward-compatible in logic — no scoring formulas or statistical methods were altered.
Bug #1 (P0): Missing
config.R— Addedconfig.R.exampledocumenting every required variable. Scripts now print a clear message whenconfig.Ris absent, pointing to the example.Bug #2 (P2): Misleading column name — Renamed
winner_single_only_fraction→winner_no_both_fractionin03_run_priority_audit.Rto match theany(evidence_type=="both")logic.Bug #3 (P2): Wrong method name — Renamed
RankProduct_missing0→RankGeometricMean_missing0. The formulasqrt(rank1 * rank2)is geometric mean, not rank product. Also removed redundantpmax(...,0).Bug #4 (P1): Duplicated
average_precision_grouped— Extracted toscripts/utils.R;01and02now source it from there.Bug #5 (P1): Triplicated normalisation helpers — Extracted
normalize_gene,normalize_tissue,read_gmt_genes,load_rda_listtoscripts/utils.R.Bug #7 (P1): Hardcoded paths in 02/03 — Both scripts now
source("config.R")andsource("scripts/utils.R"), using sharedOUT_DIR,DIAG_OUT_DIR,AUDIT_OUT_DIR.Bug #10 (P3): Missing
.gitignore— Added, covering R artifacts and large regenerable outputs.Not changed (design choices): Bug #6 (asymmetric penalties), Bug #8 (AUC implementations), Bug #9 (bootstrap stratification).
Files:
config.R.example(new),scripts/utils.R(new), updated00/01/02/03,.gitignore(new).Note:
03_run_priority_audit.Rreads03_gene_level_scores_COMPASS_and_baselines.tsvwhich must be regenerated by01with the newRankGeometricMean_missing0column name.